Skip to content

CLI2: copy mapfile for diff statistics #15110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

CLI2: copy mapfile for diff statistics #15110

wants to merge 4 commits into from

Conversation

@JojoS62
Copy link
Contributor

@JojoS62 JojoS62 commented Sep 27, 2021

Summary of changes

copy the linker map file to target.elf.map.old. The memap.py statistics generator looks for a given filename +'.old'. If it exists, it generates the difference to previous module sizes as with CLI1.
Current behaviour is not copy the map file, so this feature is not yet available in CLI2 builds.

Impact of changes

The file(COPY_FILE, ...) function in cmake requires cmake 3.21, mbed is using currently 3.19. This PR maybe hold back until mbed upgrades to require cmake >= 3.21.

Migration actions required

Documentation

Pull request type 

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ciarmcom
Copy link
Member

ciarmcom commented Sep 27, 2021

@JojoS62, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from a team September 27, 2021 11:00
@ladislas
Copy link
Contributor

ladislas commented Sep 27, 2021

Ah that's great! I really missed this feature and ended up implementing a workaround copying both old and new map files and then using diff. It works but not as nice.

@JojoS62
Copy link
Contributor Author

JojoS62 commented Sep 27, 2021

yes, the only problem maybe that it requires a relative new cmake version. I have tried with other existing function, but without the RESULT 0 feature, the functions throw an error if the file not exists or wants to overwrite an existing one.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 27, 2021

cmake_minimum_required(VERSION 3.19.0 FATAL_ERROR)

As it requires a new version, it should update the version in the main CMakeLists. As I know, we provided at least 2 fixes in CMake for armclang, it would be nice to have them. 3.21.3 should have them

@JojoS62
Copy link
Contributor Author

JojoS62 commented Sep 27, 2021 •
edited
Loading

a quick search shows that it will affect >100 CMakeLists.txt, a lot in the tests. I guess there is no common setting and all need to be updated?

I will try.

@mergify mergify bot added the needs: work label Sep 28, 2021
@mergify
Copy link

mergify bot commented Sep 28, 2021

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@mergify mergify bot removed the needs: review label Sep 28, 2021
@JojoS62
Copy link
Contributor Author

JojoS62 commented Sep 28, 2021

closing because of problems with rebase, I will create a new PR

@JojoS62 JojoS62 closed this Sep 28, 2021
@JojoS62 JojoS62 mentioned this pull request Sep 28, 2021
Merged
@JojoS62 JojoS62 deleted the fix-memmap-diff branch September 28, 2021 15:12
@JojoS62
Copy link
Contributor Author

JojoS62 commented Sep 28, 2021

replaced by #15117 due to rebase problems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JojoS62 @ciarmcom @ladislas @0xc0170